Skip to content

Allow media type to be specified in release asset upload #1102

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

aidenkeating
Copy link
Contributor

Update UploadOptions with a MediaType field. If the MediaType is
provided to UploadReleaseAsset it will take precedence over the type
retrieved from looking at the file extension. The complete order of
precedence is:

  • MediaType in UploadOptions
  • media type from file extension
  • default media type, application/octet-stream

Update unit tests to test the order of precedence, default value and
that the Content-Type header is actually set in the upload request.

Update UploadOptions with a MediaType field. If the MediaType is
provided to UploadReleaseAsset it will take precedence over the type
retrieved from looking at the file extension. The complete order of
precedence is:

- MediaType in UploadOptions
- media type from file extension
- default media type, application/octet-stream

Update unit tests to test the order of precedence, default value and
that the Content-Type header is actually set in the upload request.
@googlebot googlebot added the cla: yes Indication that the PR author has signed a Google Contributor License Agreement. label Jan 22, 2019
@gmlewis gmlewis requested a review from gauntface January 23, 2019 12:30
Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice, @aidenkeating! I like this a lot! Thank you for doing this.
LGTM.

Awaiting second LGTM before merging.

Copy link
Contributor

@gauntface gauntface left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything else looks great, thanks for the PR.

@gmlewis
Copy link
Collaborator

gmlewis commented Jan 25, 2019

For other Gophers watching, @aidenkeating has taken advantage of a cool feature of the JSON Marshaling machinery... this doc says:

As a special case, if the field tag is "-", the field is always omitted. Note that a field with name "-" can still be generated using the tag "-,".

Since we don't want MediaType to be sent to the GitHub server, we used the "-" field tag, which is super-handy in this case.

Thank you, @gauntface!
Merging.

@gmlewis gmlewis merged commit 10ee8e0 into google:master Jan 25, 2019
@aidenkeating
Copy link
Contributor Author

@gmlewis @gauntface Thanks for the review, really appreciated!

I should have documented why - is being used, @gmlewis thanks for the detailed explanation.

@gmlewis
Copy link
Collaborator

gmlewis commented Jan 25, 2019

No worries at all, @aidenkeating!

This repo attracts a lot of new-to-Go developers since @willnorris laid an excellent foundation and maintained excellent consistency throughout the large API surface... so I try to remember this when anyone ventures into less-frequently-used language constructs... and yours is a great example.

Thanks again for the PR!

n1lesh pushed a commit to n1lesh/go-github that referenced this pull request Oct 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Indication that the PR author has signed a Google Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants